Add lookupOpenMiningRoundByNumber#5060
Add lookupOpenMiningRoundByNumber#5060adetokunbo wants to merge 3 commits intodfordivam/cip-104-sv-app-rewards-feature-branchfrom
Conversation
73d2991 to
470a2d3
Compare
d8096d6 to
54a0a93
Compare
| ): Future[Seq[ContractWithState[OpenMiningRound.ContractId, OpenMiningRound]]] = | ||
| tcsStore.listAllContractsAsOf(OpenMiningRound.COMPANION, asOf) | ||
|
|
||
| override def lookupOpenMiningRoundByNumber( |
There was a problem hiding this comment.
This should at least make use of acsStore.waitUntilAcsIngested.
It may be even better to wait if the round being looked up has not been ingested yet? we could compare max round in active table to assess that, and use acsStore.waitUntilRecordTimeReached?
If doing wait here is not possible, then it may be better to return something other than None?
If None is an acceptable response in such cases, (because the trigger would try again after a while?), then it should be made clear in comments.
There was a problem hiding this comment.
The scenario would happen when scan has restarted and is catching up, The CalculateRewardsV2 would come from ScanStore which may be ahead of DbScanRewardsReferenceStore.
Currently it seems that the only thing the trigger would do when receiving None is retry?
it won't give up right?
so doing retry at the trigger level may be fine
There was a problem hiding this comment.
Definitely add acsStore.waitUntilAcsIngested, the call might throw an exception otherwise ("using a storeId before it was initialized").
IMO it's fine to have ACS stores return data for the current state of the store (i.e., return None if the mining round has not been ingested yet), and let callers handle the fact that different stores might be lagging behind by arbitrary amounts of time.
As for triggers, after retrieveTasks() returns a task, the trigger will retry the task a few times, but if it keeps failing the trigger will eventually give up and retrieve the next task.
As long as you make sure retrieveTasks() is implemented such that it properly resumes from the persisted state, you should be fine.
I would still make sure we are not failing tasks just because some store is not caught up. Tasks that fail even after retries produce log warnings, and we alert on those in production. So either have retrieveTasks() only return tasks that have a reasonable chance of succeeding, or successfully complete a task if some data is missing.
There was a problem hiding this comment.
Fixed: Thanks for all the feedback here, I've added waitUntilAcsIngested as recommended
| ): Future[Seq[ContractWithState[OpenMiningRound.ContractId, OpenMiningRound]]] = | ||
| tcsStore.listAllContractsAsOf(OpenMiningRound.COMPANION, asOf) | ||
|
|
||
| override def lookupOpenMiningRoundByNumber( |
There was a problem hiding this comment.
Definitely add acsStore.waitUntilAcsIngested, the call might throw an exception otherwise ("using a storeId before it was initialized").
IMO it's fine to have ACS stores return data for the current state of the store (i.e., return None if the mining round has not been ingested yet), and let callers handle the fact that different stores might be lagging behind by arbitrary amounts of time.
As for triggers, after retrieveTasks() returns a task, the trigger will retry the task a few times, but if it keeps failing the trigger will eventually give up and retrieve the next task.
As long as you make sure retrieveTasks() is implemented such that it properly resumes from the persisted state, you should be fine.
I would still make sure we are not failing tasks just because some store is not caught up. Tasks that fail even after retries produce log warnings, and we alert on those in production. So either have retrieveTasks() only return tasks that have a reasonable chance of succeeding, or successfully complete a task if some data is missing.
|
|
||
| create index scan_rewards_reference_store_archived_round | ||
| on scan_rewards_reference_store_archived (store_id, migration_id, round) | ||
| where round is not null; |
There was a problem hiding this comment.
I'm always nervous when we're creating indexes on non-empty tables in flyway migrations, because migrations are blocking application startup. AFAICT, this table should be sufficiently small that the index creation completes in a second, so it's fine.
8f7f2d1 to
2c49978
Compare
470a2d3 to
4b8a1b0
Compare
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
2da39fa to
9a37516
Compare
Adds this to the SV app rewards branch since it's only going to be used by this branch until it is merged into main
This change anticipates the use of this function on this branch after the computation branch is merged into main, and then merged into here.
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines